fix(numfmt):fix precision loss for large numbers in #11654#11716
Conversation
|
There's a remaining problem here: The If anyone see this, pls NEW an ISSUE if it is reasonable, I'm worrying that this PR won't be merged, thus my environment is not relyable for now. |
|
GNU testsuite comparison: |
|
I don't know if you have seen it: a few |
|
Yeah, I just woke up and correct it. |
|
GNU testsuite comparison: |
|
will the fails be a problem? How can I run these failed checks again, I guess it might not be my problem. 🤔 @cakebaker |
|
GNU testsuite comparison: |
Merging this PR will not alter performance
Comparing Footnotes
|
No, you can ignore them, they are unrelated. |
| @@ -0,0 +1,26 @@ | |||
| // This file is written to solve #11654 | |||
| // This file is related to all code changes for GitHub Issue #11654. | |||
There was a problem hiding this comment.
This sentence is more or less the same as the previous sentence, so my suggestion is to remove it.
| // This file is related to all code changes for GitHub Issue #11654. |
|
GNU testsuite comparison: |
|
GNU testsuite comparison: |
| @@ -479,14 +510,43 @@ fn consider_suffix( | |||
| Ok((v, Some((suffixes[i - 1], with_i)))) | |||
| } | |||
| } | |||
There was a problem hiding this comment.
A detail: I would add an empty line as a separator between the two functions.
| } | |
| } | |
| } | ||
| } | ||
|
|
||
| fn try_format_exact_int_without_output_scaling( |
There was a problem hiding this comment.
It looks like there is a mismatch between the function name and what the function does: the function name contains "without_output_scaling" whereas the function does some scaling. Maybe you can simply drop "without_output_scaling" from the name?
There was a problem hiding this comment.
The function can be named more precisly afer try_format_exact_int_without_suffix_scaling, is it Ok?
There was a problem hiding this comment.
Yes, it's an improvement.
|
GNU testsuite comparison: |
| fn try_scale_exact_int_without_suffix( | ||
| value: ParsedNumber, | ||
| from_unit: usize, | ||
| had_no_suffix: bool, |
There was a problem hiding this comment.
I would use a name without negation (and switch its meaning) to avoid the double negation on line 378.
There was a problem hiding this comment.
Yes, that is a problem. I'm thinking to verify had_no_suffix in transform_from() , and change name of the function try_scale_exact_int_without_suffix into try_scale_exact_int_with_from_unit.
There was a problem hiding this comment.
Yes, that might be an option.
|
GNU testsuite comparison: |
|
can my code be merged now? |
|
GNU testsuite comparison: |
|
GNU testsuite comparison: |
|
Thanks! |
Fixes #11654
numeric.rsto preserve precision for large integersformat.rsformat.rsf64and is worse than GNUcargo test --featuresunix passes for numfmt